Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle user create race condition #20097

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Apr 23, 2020

Fixes #20041

When a user's credentials are used to access ManageIQ for the first time, if properly configured, the user record will be created in the ManageIQ database.

It is possible when multiple simultaneous attempts to access ManageIQ for the first time with a single user name that a race condition could result in multiple user records being created for the same user.

This PR addresses this race condition by placing a lock on the User table when attempting
to create a new user. If a duplicate user record is found at this time it means a different simultaneous login attempt has been completed before the current one. To avoid raising a duplicate userid error this condition is rescued and handled by attempting to update, the existing user record instead of creating a new on.

Steps for Testing/QA [Optional]

Configure ManageIQ for external auth, with get user groups from Identity Provider selected.
Perform multiple simultaneous attempts to access the ManageIQ API with a username and password for a user that is not yet in the database.

This can be done with a shell script that issues multiple simultaneous curl commands to query ManageIQ.

e.g.: Repeat the below line multiple times in a shell script and run the shell script simultaneously from multiple shell windows

curl --user test_user:<PW> -k -X GET -H "Accept: application/json" https:/my_miq.example.com:443/api &

@jvlcek
Copy link
Member Author

jvlcek commented Apr 23, 2020

@miq-bot add_label bug
@miq-bot add_label authentication
@miq-bot assign @abellotti

@miq-bot miq-bot added the bug label Apr 23, 2020
@miq-bot
Copy link
Member

miq-bot commented Apr 23, 2020

@jvlcek Cannot apply the following label because they are not recognized:

  • authentication

All labels for ManageIQ/manageiq: https://github.com/ManageIQ/manageiq/labels

@jvlcek
Copy link
Member Author

jvlcek commented Apr 23, 2020

@bdunne Please review

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. good way to setup the testing.

@jvlcek jvlcek force-pushed the multi_user_race_ISSUE_20041 branch from a3301c3 to 49e37d0 Compare April 23, 2020 20:48
@miq-bot
Copy link
Member

miq-bot commented Apr 23, 2020

Checked commit jvlcek@49e37d0 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@@ -137,7 +137,18 @@ def authorize(taskid, username, *args)
end

user.lastlogon = Time.now.utc
user.save!
if user.new_record?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we only lock on a new user, which should ideally be less common than just logging in. This is great!

@Fryguy Fryguy merged commit 5906889 into ManageIQ:master Apr 24, 2020
@Fryguy Fryguy assigned Fryguy and unassigned abellotti Apr 24, 2020
@Fryguy
Copy link
Member

Fryguy commented Apr 24, 2020

@jvlcek @abellotti I think this bug has been around a while...should this be backported to ivanchuk as well?

@jvlcek
Copy link
Member Author

jvlcek commented Apr 24, 2020

@Fryguy Thank you for merging. Yes it probably should go to ivanchuck as well.

@jvlcek
Copy link
Member Author

jvlcek commented Apr 24, 2020

@miq-bot add_label Ivanchuk/yes

simaishi pushed a commit that referenced this pull request May 1, 2020
Handle user create race condition

(cherry picked from commit 5906889)
@simaishi
Copy link
Contributor

simaishi commented May 1, 2020

Jansa backport details:

$ git log -1
commit 31cbbcf94e8b0c974202494f7e93f8925ef07874
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Apr 24 13:37:41 2020 -0400

    Merge pull request #20097 from jvlcek/multi_user_race_ISSUE_20041

    Handle user create race condition

    (cherry picked from commit 5906889e13d8670376e9f7624457dac8abf7ba0b)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Auth] User Creation Race Condition
7 participants